Added High Speed Encoders Phidgets Drivers#23
Added High Speed Encoders Phidgets Drivers#23geoffviola wants to merge 9 commits intoCCNYRoboticsLab:masterfrom
Conversation
mintar
left a comment
mintar
left a comment
There was a problem hiding this comment.
Thanks for your contribution! In general, I am for merging this after the changes have been made. Before merging, please rebase + squash. It's unfortunate that I don't have the hardware to test this, but it looks like a valuable contribution.
| int32 index | ||
| int32 count | ||
| int32 count_change | ||
| int32 time |
There was a problem hiding this comment.
To conform with ROS naming standards, this message should be called EncoderParams.
There was a problem hiding this comment.
Also, I think it would be better to use a sensor_msgs/JointState message instead and remove this message altogether.
There was a problem hiding this comment.
I'm not familiar with the JointState Message. It might fit, but it is technically for "torque controlled joints". The encoder is not necessarily for that.
The EncoderParms message is legacy from the old package. It would require an extra parameter to convert the counts to an angle, but seems like a little bit too much work for now.
| { | ||
| nh.getParam("serial_number", serial_number); | ||
| } | ||
| ROS_INFO("frame_id = %s", frame_id.c_str()); |
There was a problem hiding this comment.
This prints the frame_id before it is read from the parameter server (line 222).
| phidgets_high_speed_encoder::encoder_params e; | ||
| e.header.stamp = ros::Time::now(); | ||
| e.header.frame_id = frame_id; | ||
| e.header.seq = sequence_number++; |
There was a problem hiding this comment.
This line is unnecessary and should be removed. The publisher increments the sequence number automatically.
| e.index = Index; | ||
| e.count = (inverted ? -Position : Position); | ||
| e.count_change = (inverted ? -RelativePosition : RelativePosition); | ||
| e.time = Time; |
There was a problem hiding this comment.
This should be converted to ROS time and be put in the header.stamp instead. For an example, see phidgets_imu.
There was a problem hiding this comment.
The IMU is a little different, because it publishes periodically. The encoder only sends message, when there is at least one tick.
I noticed that the phidgets IMU resynchronizes if it is too old: https://github.com/ccny-ros-pkg/phidgets_drivers/blob/master/phidgets_imu/src/imu_ros_i.cpp#L180. I'm not sure if there is still value in using the devices time, when it resynchronizes often.
There was a problem hiding this comment.
Yes, I think you're right. I was mainly thinking that having both the time and header.stamp fields is redundant. You've removed the time field now, and that's fine by me.
|
@mintar thanks for reviewing the PR! I haven't got a phidgets encoder to test either. In fact, I don't even have my IMU (it's probably inside one of the boxes from my last move - or so I hope). @geoffviola do you think you could make the changes suggested above? If not, just ping me and I can try to do it. Thanks for this PR by the way! 👍 |
|
Hi @geoffviola , thanks for responding so quickly to my comments even though your PR is now already 5 months old!
Yeah, I have no idea why the comments in
I still think this has to be changed before merging. The We should fix that now, because everything else can easily be fixed later, but changing the public API is always a pain. If it's too much work, tell @muhrix, he offered to help. |
|
Yes let me know, I'm happy to help! |
|
This PR is now continued in ros-drivers/phidgets_drivers#15. |
3 participants
Modified this code from an older driver. It fills in the header information, now. Tested it in Indigo.